fix(analytics): validate against incomplete/invalid app_remove events #1738
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a solid fix for a crash caused by malformed user properties in analytics events. The approach is robust, adding defensive checks in unwrapValueAsString and filtering invalid properties during event construction. The addition of a new test case that covers various invalid inputs is excellent for ensuring correctness and preventing future regressions. I have one suggestion to improve the readability of the new filtering logic.
| .filter(([, v]) => { | ||
| if (v === null || v === undefined) { | ||
| return false; | ||
| } | ||
| if (typeof v !== "object") { | ||
| return false; | ||
| } | ||
| const vObj = v as Record<string, unknown>; | ||
| // Filter out empty objects | ||
| if (Object.keys(vObj).length === 0) { | ||
| return false; | ||
| } | ||
| // Filter if 'value' property exists but is null/undefined/empty object | ||
| if ("value" in vObj) { | ||
| const value = vObj.value; | ||
| if ( | ||
| value === null || | ||
| value === undefined || | ||
| (typeof value === "object" && value !== null && Object.keys(value).length === 0) | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| }) |
There was a problem hiding this comment.
The filtering logic here is comprehensive but also quite complex and spread across multiple conditional statements, which could affect readability and future maintenance. To make the logic clearer and more self-documenting, you could refactor it slightly by grouping related checks and adding comments to explain each validation step.
.filter(([, v]) => {
// Property must be a non-empty object.
if (v == null || typeof v !== "object") {
return false;
}
if (Object.keys(v).length === 0) {
return false;
}
// If 'value' field exists, it must not be null, undefined, or an empty object.
if ("value" in v) {
const value = (v as { value: unknown }).value;
if (value == null || (typeof value === "object" && value !== null && Object.keys(value).length === 0)) {
return false;
}
}
return true;
})
This draft PR is a proposed solution to resolve #1712
Problem
When an analytics event payload contains malformed user properties, the SDK throws a
TypeErrorbefore the user's callback can execute:Error:
TypeError: Cannot convert undefined or null to object
at Function.keys ()
at unwrapValueAsString (analytics.js:206)
Solution